-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ReadyReplicas metric to deployment metric family #1534
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @akshitgrover! |
* Add ReadyReplicas field in Deployment status to deployment metrics family * Depicts number of readyReplicas across all replicas sets owned by the deployment Signed-off-by: Akshit Grover <akshit.grover2016@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @akshitgrover. Besides the name of the metric, everything else looked already quite good.
Signed-off-by: Akshit Grover <akshit.grover2016@gmail.com>
LGTM, cc @kubernetes/kube-state-metrics-maintainers |
Can you also update https://github.com/kubernetes/kube-state-metrics/blob/master/docs/deployment-metrics.md to include this metric? |
Signed-off-by: Akshit Grover <akshit.grover2016@gmail.com>
Docs updated 👍🏽 |
/approve |
@mrueg @fpetkovski |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, one suggestion. Otherwise lgtm.
@@ -169,6 +174,7 @@ func TestDeploymentStore(t *testing.T) { | |||
kube_deployment_status_replicas_unavailable{deployment="depl2",namespace="ns2"} 0 | |||
kube_deployment_status_replicas_updated{deployment="depl2",namespace="ns2"} 1 | |||
kube_deployment_status_replicas{deployment="depl2",namespace="ns2"} 10 | |||
kube_deployment_status_replicas_ready{deployment="depl2",namespace="ns2"} 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ready always the same as kube_deployment_status_replicas_available? Judging by these tests at least? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilic
Available and Ready replicas count will not necessarily be same
Available replicas are more like a subset of ready replicas (Replicas which are ready for a certain amount of time configurable based on spec.minReadySeconds)
Change status for kube_deployment_status_replicas_ready to EXPERIMENTAL
@lilic @fpetkovski @mrueg |
/lgtm Thanks for the contribution 👍 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshitgrover, fpetkovski, mrueg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thank you @fpetkovski |
* [FEATURE] Add --use-apiserver-cache flag to set resourceVersion=0 for ListWatch requests kubernetes#1548 * [FEATURE] Introduce metrics for Kubernetes object annotations kubernetes#1468 * [FEATURE] Introduce start time metric for containers in terminated state kubernetes#1519 * [FEATURE] Introduce metrics for cronjob job history limits kubernetes#1535 * [FEATURE] Add system_uuid dimension to kube_node_info metric kubernetes#1535 * [FEATURE] Add available replica metric for statefulsets kubernetes#1532 * [FEATURE] Add ready replica metric for deployments kubernetes#1534 * [CHANGE] Update go clients for Kubernetes to support 1.22 kubernetes#1545 * [CHANGE] Use new promlint package and update prometheus cli to 2.28.1 kubernetes#1531
Signed-off-by: Akshit Grover akshit.grover@appdynamics.com
What this PR does / why we need it:
Adds ReadyReplicas field in Deployment status to deployment metrics
Ready Replicas count is an insightful metric as it depicts the number of traffic serving pods for a particular deployment, This can be used in number of ways to compare across the board for troubleshooting
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Increases cardinality as it add another metric in the deployment metrics family
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1522